This is alternative implementation of #30896

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.

Could anyone take a look?

Could anyone take a look?

needs rebase and resolve conflicts

needs rebase and resolve conflicts

Done

@hussein-awala can you take a look?

So?

This pr will solve the problem.
But isnt this solution is better? #30896
Because in the current solution we will add git-ssh even though we don't need it.

even though we don't need it

Please describe this in more details.

I would strongly discourage using both git-sync and persistence. I am very close to propose (I did have a PR in the past about it) such combo should be forbidden because it leads to very brittle behaviour and you might have alot of problems with it. So I would rather not make it easier for people using it, to be honest.

I still failed to see where it might bring any use and there are multiple problems such setup might cause - depending on - for example - whether your persistence solution has provisioned IOPS and how "distributed" your setup is and what kind of the PVC you configure.

You can read more about it here: https://airflow.apache.org/docs/helm-chart/stable/manage-dags-files.html#notes-for-combining-git-sync-and-persistence

Also if you would like to read more and see if your solution might work better (depending on the way which PVC you are using and what kind of K8S you run and how distributed your cluster is) you can read about it in my blog post where I explained why combining Persistence and Git Sync is a bad idea:

https://medium.com/apache-airflow/shared-volumes-in-airflow-the-good-the-bad-and-the-ugly-22e9f681afca

Generally you are shooting yourself in the foot by enabling both at the same time.

I am not sure we want to fix anything about git-sync + persistence combo to be honest. I don't thinlk we should make it easier for our users to shoot htemselves in their foot. I'd rather see a PR disabling this combo.

I'd rather prefer to show some kind of warning in such a case than completely forbid it. Some users may successfully use this combination, there is no statistics related to this

I'd rather prefer to show some kind of warning in such a case than completely forbid it. Some users may successfully use this combination, there is no statistics related to this

Yes. That was also my initial thought and PR, but I could not find a way how to show warning to helm users in this case - there seem to be no good way of passing any warning informatiob to users for combination of parameters in the template that should be avoided.

Do you have some idea how to do it @dolfinus ? Or maybe even you would like to submit a PR doing it ?

No idea, and I'm not interested in this kind if solution.

No idea, and I'm not interested in this kind if solution.

Pity. You loose opportunity to become one of 2700 contributors to Airflow.

You loose opportunity to become one of 2700 contributors to Airflow.

I already am.

If maintainers are not interested of merging PR which avoids producing wrong pod description, and instead prefer to do nothing with that for months - ok, but very pitty to hear.

If maintainers are not interested of merging PR which avoids producing wrong pod description, and instead prefer to do nothing with that for months - ok, but very pitty to hear.

Well. Maintainers are interested in getting future looking solution that not only solve this problem but will prevent users from shooting themselves in their foot and then opening even more issues when the do and have weird problems. Sometimes putting a small patch where your blood is poisoned, will make you die anyway, but you will not realize it because you turn a blind eye and pretend that tbe bigger problem does not exist.

So -if this PR (which I hope it will) will start a discussion that will lead to a better solution, that might be the best outcome of this PR. Merging it now without discussion will simply let people turn a blind eye on the real issue we have, so I would strongly suggest to discuss alternatives and see what other proposals people have knowing the context.

If you think we are doing nothing here, you are completely wrong:

• I initially proposed to disable this combo: Disable GitSync + Persistence combo in the Helm Chart #28822 - but then it was not well received
• Then I closed in favour of documentation STRONG discouragement of using this combo: Update git-sync description in Helm Chart documentation #32181

Yet - despite the very explicit and strong warning - people like you continue ignoring the strong discouragement we have and raise issues about some of the things not working with this combo - which of course are unrelated, but they are - again - turning a blind eye not realising they have a bigger problem and that what they do is really poisonous blood in their organism that they are trying to put patch on.

So yeah. I am not thinking about doing nothing. I am looking for other's comments and suggestions what we do in this situation - including taking your suggestion about warning (which I have no idea how to do even if I tried in the original PR).

It's a pity that some users are complaining, when the maintainers actually go to a great length to solve the actual problems rather than let their users to patch and turn blind eye on the bigger problems they have. I'd say it would be an easy solution to just merge the PR. yet I've chosen a much more demanding approach that could help to solve bigger problem if we have people helping here.

What exactly are we waiting here?

We are waiting to hear from others.

If nobody is interested in this, I don't see any reason to keep this open

we are interested in this

we are interested in this

Feel free to create PR and lead it completion then @amoskueez . Nothing more is needed here.

Second try: #36628